-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(sveltekit): Add Compatibility for builtin SvelteKit Tracing #17423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
sentryHandle
c478f9d
to
bd930ce
Compare
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just leaving a couple of comments!
packages/sveltekit/src/server/sdk.ts
Outdated
const defaultIntegrations = [...getDefaultNodeIntegrations(options), rewriteFramesIntegration()]; | ||
|
||
const config = getKitTracingConfig(); | ||
if (config.instrumentation) { | ||
// Whenever `instrumentation` is enabled, we don't need httpIntegration to emit spans | ||
// - if `tracing` is enabled, kit will emit the root span | ||
// - if `tracing` is disabled, our handler will emit the root span | ||
// In old (hooks.server.ts) based SDK setups, adding the default version of the integration does nothing | ||
// for incoming requests. We still add it in default config for the undocumented case that anyone is | ||
// using the SDK by `--import`ing the SDK setup directly (e.g. with adapter-node). | ||
defaultIntegrations.push(httpIntegration({ disableIncomingRequestSpans: true })); | ||
if (config.tracing) { | ||
// If `tracing` is enabled, we need to instrument spans for the server | ||
defaultIntegrations.push(svelteKitSpansIntegration()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this works right now but I'm not sure if it will in SvelteKit 3 when we remove the experimental flag -- we plan on keeping a flag for tracing (you probably don't want SvelteKit emitting spans / including @opentelemetry/api
in your bundle if you're not planning on using it), but the instrumentation flag is going to go away entirely -- users will either create an instrumentation file or not create one, and that signals to us whether or not we need to load it. It only has a flag right now so that people have to explicitly opt into the experimental nature (in case we need to make breaking changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good context, thanks Elliot!
but the instrumentation flag is going to go away entirely -- users will either create an instrumentation file or not create one
that makes sense of course. Didn't think of it but only expected the option to be lifted outside of experimental. In this case I think we can just be a bit more "naive" and avoid the conditional logic here. The only thing that will be problematic is the "undocumented" case of using the SDK with --import
but not using Kit's tracing/instrumentation. Which I think we can ignore, given it's undocumented 😅
Which also means we likely don't need to inject the config at build time into the files at all 🎉 (we'll still need it at build time but for now I think that's fine).
* Experimental tracing and instrumentation config is available | ||
* @since 2.31.0 | ||
*/ | ||
type BackwardsForwardsCompatibleKitConfig = Config['kit'] & { experimental?: SvelteKitTracingConfig }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love the name 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! 🚀
if (sentryVitePluginsOptions) { | ||
const sentryVitePlugins = await makeCustomSentryVitePlugins(sentryVitePluginsOptions); | ||
if (mergedOptions.autoUploadSourceMaps) { | ||
sentryPlugins.push(await makeGlobalValuesInjectionPlugin(svelteConfig, mergedOptions)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use a comment: why is this plugin added when autoUploadSourceMaps is set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While writing this comment, I realized we should revisit if we even have to do this. All of this frames rewriting comes from pre-debugId times. For now I decided to leave it as-is, just to not change behaviour. But left a TODO to revisit this.
*/ | ||
export function svelteKitSpansIntegration(): Integration { | ||
return { | ||
name: 'SvelteKitSpansEnhancment', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name: 'SvelteKitSpansEnhancment', | |
name: 'SvelteKitSpansEnhancement', |
// Using preprocessEvent to ensure the processing happens before user-configured | ||
// event processors are executed | ||
preprocessEvent(event) { | ||
if (event.type === 'transaction') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed we could check if the root span comes from sveltekit here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized this doesn't work on cloudflare because for some reason the sveltekit.handle.root
span isn't emitted.
So our Cloudflare SDK emits the http.server
span. Not sure why but I think this is something to follow up on. For now I wanna get a basic, working version out.
…n, change root span enhancement time
e27f7cc
to
87c4109
Compare
This PR updates our SvelteKit SDK to be compatible with Sveltekit's new tracing feature, while maintaining backwards compatibility with Sentry<>SvelteKit setups on previous Kit versions or when SvelteKit's tracing feature is not used.
Nothing will change for users who use SvelteKit and Sentry without builtin SvelteKit tracing support.
If Kit tracing is used, the SDK
http.server
span but uses Kit'ssveltekit.handle.root
span as thehttp.server
root span.httpIntegration
to avoid emitting a span for the incoming request span (this is in line with NextJS and Remix)http.server
name conventionssveltekit.tracing.original_name
attributeTo detect at runtime, if Kit-native tracing is used, I had to make some build time adjustments
svelte.config.js
load
functions only for client-side code, if kit tracing is enabledMisc
Follow-up items to this PR
sentryHandle
. Decided to tackle this later since a) the feature is experimental and b) this PR is already too large. Sorry reviewers 😬instrumentation.server.ts
and the server is shut down. We can fix this by listening to theprocess.on('sveltekit:shutdown')
event.see sveltejs/kit#13899
ref #16982
cc @elliott-with-the-longest-name-on-github